Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cpp-to-wasm test #968

Merged
merged 6 commits into from
Aug 25, 2021
Merged

Add cpp-to-wasm test #968

merged 6 commits into from
Aug 25, 2021

Conversation

Manishearth
Copy link
Member

This is progress towards #967 , allowing others to work on debugging the issues here.

This compiles, but I have not yet added it to CI since it's not working correctly.

The current state shows the following warning:

[14:45:04] मanishearth@manishearth-glaptop ~/dev/icu4x/ffi/cpp/examples/fixeddecimal ^_^ 
$ make main.html 
emcc -std=c++17 test.cpp ../../../../target/wasm32-unknown-unknown/debug/libicu_capi.a -ldl -lpthread -lm -DWASM -g  -o main.html --emrun -sWASM=1
wasm-ld: warning: function signature mismatch: ICU4XFixedDecimalFormat_try_new
>>> defined as (i32, i32, i32, i32) -> void in /tmp/emscripten_temp_r7m6oimw/test_0.o
>>> defined as (i32, i32, i32, i32, i32) -> void in ../../../../target/wasm32-unknown-unknown/debug/libicu_capi.a(icu_capi.rpa9bibmika8fph.rcgu.o)

This seems to be a return type thing (cc @shadaj, if you're interested / have time). Might be worth trying to switch to DiplomatResult, but either way the struct should work.

It's possible that my way of linking things is not ideal either.

@Manishearth Manishearth requested a review from a team as a code owner August 18, 2021 21:48
@Manishearth
Copy link
Member Author

Manishearth commented Aug 18, 2021

The error in the PR body might be a red herring, I don't get the same error when I apply this to the pluralrules example, however I do still get the error at runtime.

Update: I've fixed the allocation issue. The layout issue remains.

original bug details The runtime error (for both files) is:
exception thrown: RuntimeError: unreachable,RuntimeError: unreachable
    at rust_oom (http://localhost:8000/main.wasm:wasm-function[3809]:0xc4d1c)
    at __rg_oom (http://localhost:8000/main.wasm:wasm-function[3864]:0xc59f2)
    at __rust_alloc_error_handler (http://localhost:8000/main.wasm:wasm-function[1244]:0x37b94)
    at alloc::alloc::handle_alloc_error::h210cb584d6b73817 (http://localhost:8000/main.wasm:wasm-function[3863]:0xc59e7)
    at alloc::alloc::exchange_malloc::h343802ddc1712c95 (http://localhost:8000/main.wasm:wasm-function[479]:0xd16b)
    at icu_capi::locale::ffi::ICU4XLocale::create::_$u7b$$u7b$closure$u7d$$u7d$::had3acf8706863d90 (http://localhost:8000/main.wasm:wasm-function[687]:0x1ca6a)
    at core::option::Option$LT$T$GT$::map::h2c81b5b50bfffbba (http://localhost:8000/main.wasm:wasm-function[774]:0x256d9)
    at icu_capi::locale::ffi::ICU4XLocale::create::h136ddd7fdbcb961d (http://localhost:8000/main.wasm:wasm-function[465]:0xcb05)
    at ICU4XLocale_create (http://localhost:8000/main.wasm:wasm-function[467]:0xcc96)
    at ICU4XLocale::create(std::__2::basic_string_view<char, std::__2::char_traits<char> >) (http://localhost:8000/main.wasm:wasm-function[16]:0x2647)

Uncaught RuntimeError: unreachable
    at rust_oom (main.wasm:0xc4d1c)
    at __rg_oom (main.wasm:0xc59f2)
    at __rust_alloc_error_handler (main.wasm:0x37b94)
    at alloc::alloc::handle_alloc_error::h210cb584d6b73817 (main.wasm:0xc59e7)
    at alloc::alloc::exchange_malloc::h343802ddc1712c95 (main.wasm:0xd16b)
    at icu_capi::locale::ffi::ICU4XLocale::create::_$u7b$$u7b$closure$u7d$$u7d$::had3acf8706863d90 (main.wasm:0x1ca6a)
    at core::option::Option$LT$T$GT$::map::h2c81b5b50bfffbba (main.wasm:0x256d9)
    at icu_capi::locale::ffi::ICU4XLocale::create::h136ddd7fdbcb961d (main.wasm:0xcb05)
    at ICU4XLocale_create (main.wasm:0xcc96)
    at ICU4XLocale::create(std::__2::basic_string_view<char, std::__2::char_traits<char> >) (main.wasm:0x2647)

which sounds like i need to set up memory infra somehow

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • Cargo.lock is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2021

Codecov Report

Merging #968 (f604952) into main (b66b237) will increase coverage by 0.11%.
The diff coverage is n/a.

❗ Current head f604952 differs from pull request most recent head 6fbeb50. Consider uploading reports for the commit 6fbeb50 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #968      +/-   ##
==========================================
+ Coverage   74.89%   75.00%   +0.11%     
==========================================
  Files         221      216       -5     
  Lines       12811    12737      -74     
==========================================
- Hits         9595     9554      -41     
+ Misses       3216     3183      -33     
Impacted Files Coverage Δ
experimental/provider_ppucd/src/parse_ppucd.rs 93.27% <0.00%> (-0.14%) ⬇️
provider/testdata/src/paths.rs
provider/testdata/src/metadata.rs
provider/testdata/src/fs.rs
provider/testdata/src/lib.rs
provider/testdata/src/blob.rs

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b66b237...6fbeb50. Read the comment docs.

@coveralls
Copy link

coveralls commented Aug 18, 2021

Pull Request Test Coverage Report for Build 88639083c982d3f0dc657a4830b1e8ac01848d05-PR-968

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.008%) to 75.048%

Files with Coverage Reduction New Missed Lines %
experimental/provider_ppucd/src/parse_ppucd.rs 1 93.28%
Totals Coverage Status
Change from base Build b66b237d22db389c8333c558937e66e16d12db34: -0.008%
Covered Lines: 9685
Relevant Lines: 12905

💛 - Coveralls

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • ffi/capi/Cargo.toml is different
  • ffi/capi/src/locale.rs is now changed in the branch
  • ffi/capi/src/wasm_glue.rs is now changed in the branch
  • ffi/cpp/examples/fixeddecimal-wasm/.gitignore is now changed in the branch
  • ffi/cpp/examples/fixeddecimal-wasm/main.html is now changed in the branch
  • ffi/cpp/examples/fixeddecimal-wasm/main.js is now changed in the branch
  • ffi/cpp/examples/fixeddecimal-wasm/main.wasm is now changed in the branch
  • ffi/cpp/examples/fixeddecimal-wasm/Makefile is now changed in the branch
  • ffi/cpp/examples/fixeddecimal-wasm/test.cpp is now changed in the branch
  • ffi/cpp/examples/fixeddecimal/Makefile is different
  • ffi/cpp/examples/fixeddecimal/test.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • ffi/capi/src/wasm_glue.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • ffi/capi/src/locale.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure I see where this is going yet.

@@ -0,0 +1,1300 @@
<!doctype html>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Where did this giant HTML and JS file come from? Please add a copyright header with more info, or replace it with a smaller file that you write yourself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally checked them in, they shouldn't be checked in. My bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They all come from Emscripten, to be clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emscripten is kinda annoying to use, Rust's WASM story is very good (and Rust moves its boilerplate into separate crates), emcc on the other hand produces a lot of boilerplate at once because it's nowhere near as good.

ffi/cpp/examples/fixeddecimal-wasm/test.cpp Outdated Show resolved Hide resolved
ffi/cpp/examples/fixeddecimal-wasm/Makefile Outdated Show resolved Hide resolved
fn calloc(num: usize, size: usize) -> *mut u8;
fn realloc(ptr: *mut u8, size: usize) -> *mut u8;
}
unsafe impl GlobalAlloc for WasmMalloc {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why not dlmalloc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work! I didn't bother to debug it.

Comment on lines 79 to 82
fn malloc(size: usize) -> *mut u8;
fn free(ptr: *mut u8);
fn calloc(num: usize, size: usize) -> *mut u8;
fn realloc(ptr: *mut u8, size: usize) -> *mut u8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: are these imports implemented anywhere? I don't see them in main.js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasm implements them (or at least, emcc does). When you build C/++ code with emcc it's already hooked up to an allocator, exposed as the regular malloc/free stuff.

ffi/capi/Cargo.toml Outdated Show resolved Hide resolved
@Manishearth Manishearth requested a review from sffc August 19, 2021 02:14
@Manishearth
Copy link
Member Author

Filed rust-lang/rust#88152 upstream

@Manishearth Manishearth changed the title Add non-working cpp-to-wasm test Add cpp-to-wasm test Aug 21, 2021
@Manishearth Manishearth marked this pull request as draft August 21, 2021 06:35
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .github/workflows/build-test.yml is different
  • Cargo.lock is different
  • ffi/capi/Cargo.toml is different
  • ffi/capi/src/lib.rs is no longer changed in the branch
  • ffi/capi/src/malloc_glue.rs is no longer changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/.gitignore is different
  • ffi/cpp/examples/fixeddecimal_wasm/Makefile is different
  • ffi/cpp/examples/fixeddecimal_wasm/module.js is now changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/module.wasm is now changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/node-test.js is now changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/package.json is now changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/README is now changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/test.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • ffi/cpp/examples/fixeddecimal_wasm/module.js is no longer changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/module.wasm is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth
Copy link
Member Author

Manishearth commented Aug 21, 2021

I finally got it working. Aside from the diplomat update, it should be reviewed afresh, a lot has changed (hence the force-pushes, but I also wanted to get rid of files that got accidentally checked in)

Marking as a draft since this won't work until serde-rs/serde#2076 lands anyway. Also I need to get the WASM CI working (and convert this into cargo-make tasks)

denylist = ["bench", "wearos", "freertos", "x86tiny", "smaller_static"]

[lib]
crate-type = ["staticlib", "rlib", "cdylib"]
crate-type = ["staticlib", "rlib"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dylib had to be removed because it doesn't work for emscripten (without what seems to be a large amount of extra effort),

we're not using cdylibs anyway, and it's still possible to compile icu_capi to a cdylib, just not in the emscripten context, and unfortunately rust-lang/cargo#4881 doesn't exist.

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth
Copy link
Member Author

Manishearth commented Aug 21, 2021

Finally works! It's still not mergeable because of the serde patch, but it's ready for review! cc @sffc

(I squashed up most of the CI commits because I was trying a lot of things that didn't work)

@Manishearth Manishearth marked this pull request as ready for review August 21, 2021 20:04
@Manishearth
Copy link
Member Author

And we have a new serde version!

Usually when you are relying on a newer version of a crate you should specify that version in Cargo.toml (not just Cargo.lock) so that users will also pull in at least that version. However in this case we need the dependency bump only for a single CI test for a strange platform, and the updated dependency is something users of that platform will need anyway, so I only changed Cargo.lock.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .github/workflows/build-test.yml is different
  • Cargo.lock is different
  • ffi/capi/Cargo.toml is different
  • ffi/capi/include/result_void_ICU4XLocaleError.h is no longer changed in the branch
  • ffi/capi/include/result_void_void.h is no longer changed in the branch
  • ffi/cpp/include/ICU4XFixedDecimalFormatOptions.hpp is no longer changed in the branch
  • ffi/cpp/include/ICU4XLocale.hpp is no longer changed in the branch
  • ffi/cpp/include/ICU4XLocaleCanonicalizer.hpp is no longer changed in the branch
  • ffi/cpp/include/ICU4XPluralRules.hpp is no longer changed in the branch
  • ffi/cpp/include/result_void_ICU4XLocaleError.h is no longer changed in the branch
  • ffi/cpp/include/result_void_void.h is no longer changed in the branch
  • Makefile.toml is different
  • tools/scripts/ffi.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth
Copy link
Member Author

Rebased over #974

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@@ -229,7 +229,11 @@ jobs:
- name: Load nightly Rust toolchain for WASM.
run: |
rustup install nightly-2021-02-28
rustup install nightly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Pin a version of nightly because nightly versions are frequently flaky.

While you're at it, try bumping the 2021-02-28 version to see if that test starts running again in a newer nightly. Then maybe you can use the same, newer nightly for both wasm32-unknown-unknown and wasm32-unknown-emscripten.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice! I'll try

.github/workflows/build-test.yml Show resolved Hide resolved
@Manishearth
Copy link
Member Author

error: WASM error: Invalid name type (at offset 18707)

@Manishearth
Copy link
Member Author

unknown name subsection at 1187412
unknown name subsection at 1187432

@Manishearth
Copy link
Member Author

Okay, so updating the nightly doesn't work, pinning everything to nightly-2021-02-28. We should try to do a followup to fix things, because once rust-lang/rust#85499 lands we'll want to update to a rust including that so that some of the yoke bugs are fixed.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • .github/workflows/build-test.yml is different
  • ffi/capi/examples/fixeddecimal_tiny/Makefile is no longer changed in the branch
  • ffi/capi/examples/fixeddecimal_tiny/README.md is no longer changed in the branch
  • ffi/cpp/examples/fixeddecimal_wasm/Makefile is different
  • tools/scripts/ffi.toml is different
  • tools/scripts/valgrind.toml is no longer changed in the branch
  • tools/scripts/wasm.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@Manishearth Manishearth requested a review from sffc August 25, 2021 03:48
@Manishearth Manishearth merged commit 19c05d9 into unicode-org:main Aug 25, 2021
@Manishearth Manishearth deleted the wasm-cpp branch August 25, 2021 05:32
@sffc sffc linked an issue Aug 26, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test ICU4X being used via C++ compiled to WASM
4 participants